Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added Compat.AbstractDateTime #443

Merged
merged 3 commits into from
Jan 10, 2018
Merged

Added Compat.AbstractDateTime #443

merged 3 commits into from
Jan 10, 2018

Conversation

rofinn
Copy link
Contributor

@rofinn rofinn commented Dec 30, 2017

No description provided.

@rofinn rofinn changed the title Added Compat.AbstractDateTime. Added Compat.AbstractDateTime Dec 30, 2017
ararslan
ararslan reviewed Dec 31, 2017
View reviewed changes
src/Compat.jl Outdated
@@ -769,6 +769,12 @@ else
import Dates
end

if VERSION < v"0.7.0-DEV.3216"
const AbstractDateTime = Dates.TimeType
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this (and below) be Compat.Dates rather than just Dates?

@ararslan
Copy link
Member

Cool, thanks! Could you add a test and an entry in the README?

@ararslan ararslan requested a review from omus December 31, 2017 23:42
omus
omus requested changes Jan 1, 2018
View reviewed changes
test/runtests.jl Outdated
@@ -1063,6 +1063,10 @@ end
@test Compat.notnothing(1) == 1
@test_throws ArgumentError Compat.notnothing(nothing)

# 0.7.0-DEV.3216
@test Compat.AbstractDateTime === (isdefined(Compat.Dates, :AbstractDateTime) ? Compat.Dates.AbstractDateTime : Compat.Dates.TimeType)
@test Compat.AbstractDateTime <: Compat.Dates.TimeType
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably should have a test which is Compat.Dates.DateTime <: Compat.AbstractDateTime

src/Compat.jl
else
const AbstractDateTime = Compat.Dates.AbstractDateTime
end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably should export this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems like an odd choice given that it isn't exported in base.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right here. Don't export this since it isn't exported in Base

src/Compat.jl
@@ -769,6 +769,12 @@ else
import Dates
end

if VERSION < v"0.7.0-DEV.3216"
const AbstractDateTime = Compat.Dates.TimeType
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems slightly dangerous to use with dispatch. You could accidentally be given a Date or Time. I'm not sure what a better solution would be ATM

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly just having a const AbstractDateTime = Union{DateTime, ZonedDateTime} in TimeZones.jl would be a better solution. Doing this in Compat would make TimeZones a requirement in Compat which is probably worse.

Copy link
Contributor Author

@rofinn rofinn Jan 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That could also work for our use cases, but I figured it's better to write code that is a bit too general than too restrictive. IFAICT, the worst case scenario is that someone passes in a Date or Time and the code errors at a later point... making the traceback more complicated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A possible alternative solution is to implement @compat AbstractDateTime which would re-write this as a Union{DateTime,ZonedDateTime}. That would probably be a bit too annoying though.

@omus
Copy link
Member

omus commented Jan 8, 2018

@rofinn I'm ok if we go ahead with this change. I definitely don't like how overly general the fallback type is but it should probably be ok as a temporary compatibility change.

@omus
Copy link
Member

omus commented Jan 9, 2018

Any other comments? I think this approach may be the best we can do for now. I'll merge tomorrow if there are no objections.

@omus omus merged commit 3c1d4fc into JuliaLang:master Jan 10, 2018
@omus omus deleted the rf/abstractdatetime branch January 10, 2018 16:11
martinholters added a commit that referenced this pull request Oct 1, 2019
@martinholters
Mark AbstractDateTime from #443 for deprecation
ea7ce9c
martinholters added a commit that referenced this pull request Oct 8, 2019
@martinholters
Mark AbstractDateTime from #443 for deprecation
ff6f907
martinholters added a commit that referenced this pull request Oct 13, 2019
@martinholters
Drop support for Julia versions <1.0 (#665)
4fa5519 
* Bump required Julia version to 1.0

* Remove compatibility support code for:
  * `at-__MODULE__` (from #363)
  * `devnull`, `stdin`, `stdout`, and `stderr` from #499
  * `at-nospecialize` (from #385 and #409)
  * `isabstracttype` and `isconcretetype` (from #477)
  * `invokelatest` from #424
  * array-like access to `Cmd` from #379
  * `Val(n)` and `ntuple`/`reshape` with `Val` from #381 and #399
  * `logdet(::Any)` fallback from #382
  * `chol(::UniformScaling)` from #382
  * `pushfirst!`, `popfirst!` from #444
  * `fieldcount` from #386
  * `read(obj, ::Type{String})` from #385 and #580
  * `InexactError`, `DomainError`, and `OverflowError` constructors from #393
  * `corrected` kw arg to `cov` from #401
  * `adjoint` from #401
  * `partialsort` from #401
  * `pairs` from #428
  * `AbstractRange` from #400
  * `rtoldefault` from #401
  * `Dates.Period` rounding from #462
  * `IterativeEigensolvers` from #435
  * `occursin` from #520
  * `Char` concatenation from #406
  * `BitSet` from #407
  * `diagm` and `spdiagm` with pairs from #408
  * `Array` c'tors from `UniformScaling` from #412 and #438
  * `IOContext` ctor taking pairs from #427
  * `undef` from #417 and #514
  * `get` on `ENV` from #430
  * `ComplexF...` from #431
  * `tr` from #614
  * `textwidth` from #644
  * `isnumeric` from #543
  * `AbstractDict` from #435
  * `axes` #435 and #442
  * `Nothing` and `Cvoid` from #435
  * `Compat.SuiteSparse` from #435
  * `invpermute!` from #445
  * `replace` with a pair from #445
  * `copyto!` from #448
  * `contains` from #452
  * `CartesianIndices` and `LinearIndices` from #446, #455, and #524
  * `findall` from #466 (and #467).
  * `argmin` and `argmax` from #470, #472, and #622
  * `parentmodule` from #461
  * `codeunits` from #474
  * `nameof` from #471
  * `GC` from #477
  * `AbstractDisplay` from #482
  * `bytesavailable` from #483
  * `firstindex` and `lastindex` from #480 and #494
  * `printstyled` from #481
  * `hasmethod` from #486
  * `objectid` from #486
  * `Compat.find*` from #484 and #513
  * `repr` and `showable` from #497
  * `Compat.names` from #493 and #505
  * `Compat.round` and friends #500, #530, and #537
  * `IOBuffer` from #501 and #504
  * `range` with kw args and `LinRange` from #511
  * `cp` and `mv` from #512
  * `indexin` from #515
  * `isuppercase` and friends from #516
  * `dims` and `init` kwargs from #518, #528, #590, #592, and #613
  * `selectdim` from #522 and #531
  * `repeat` from #625
  * `fetch(::Task)` from #549
  * `isletter` from #542
  * `isbitstype` from #560
  * `at-cfunction` from #553 and #566
  * `codeunit` and `thisind` and friends from #573
  * `something` from #562
  * `permutedims` from #582
  * `atan` from #574
  * `split` and `rsplit` from #572
  * `mapslices` from #588
  * `floatmin` and `floatmax` from #607
  * `dropdims` from #618
  * required keyword arguments from #586
  * `CartesianRange` in `at-compat` from #377
  * `finalizer` from #416
  * `readline`, `eachline`, and `readuntil` from #477, #541, and #575
  * curried `isequal`, `==`, and `in` from #517
  * `Some` from #435 and #563
  * `at-warn` and friends from #458

* Remove old deprecations

* Deprecate:
  * `Compat.Sockets` from #545 and #594
  * `TypeUtils` from #304
  * `macros_have_sourceloc` from #355
  * `Compat.Sys` from #380, #433, and #552
  * `Compat.MathConstants` from #401
  * `Compat.Test`, `Compat.SharedArrays`, `Compat.Mmap`, and `Compat.DelimitedFiles` from #404
  * `Compat.Dates` from #413
  * `Compat.Libdl` from #465 (and #467)
  * `AbstractDateTime` from #443
  * `Compat.Printf` from #435
  * `Compat.LinearAlgebra` from #463
  * `Compat.SparseArrays` from #459
  * `Compat.Random` from #460, #601, and #647
  * `Compat.Markdown` from #492
  * `Compat.REPL` from #469
  * `Compat.Serialization` from #473
  * `Compat.Statistics` from #583
  * `Fix2` from #517
  * `Compat.Base64` from #418
  * `Compat.Unicode` from #432 and #507
  * `notnothing` from #435 and #563
  * `Compat.IteratorSize` and `Compat.IteratorEltype` from #451
  * `enable_debug(::Bool)` from #458
  * `Compat.Distributed` from #477
  * `Compat.Pkg` from #485
  * `Compat.InteractiveUtils` from #485
  * `Compat.LibGit2` from #487
  * `Compat.UUIDs` from #490
  * `Compat.qr` from #534
  * `Compat.rmul!` from #546
  * `Compat.norm` abd friends from #577

* Remove obsolete README entry, missed in #385

* Remove obsolete tests (e.g. missed in #372)

* Remove obsolete `VERSION` conditionals and some minor clean-up
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@omus omus omus approved these changes

@ararslan ararslan ararslan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rofinn @ararslan @omus